update of DOM listeners in FilePlayer when audio/video tags was switched #234
Conversation
| const wasAudio = AUDIO_EXTENSIONS.test(prevProps.url) || prevProps.config.file.forceAudio | ||
| const isAudio = AUDIO_EXTENSIONS.test(url) || config.file.forceAudio | ||
| if (wasAudio !== isAudio) { | ||
| this.removeDOMListeners() |
There was a problem hiding this comment.
I'm not sure this is necessary. At this point, this.player is already the new <audio> or <video> element and the old one has been unmounted and removed from the DOM. We might have to do the same check in componentWillReceiveProps and run removeDOMListeners whilst the old element is still mounted? Or we might not have to run it at all, as the element is being killed anyway.
There was a problem hiding this comment.
Yes, I was trying to set this check in cwrp, but by unknown (for me) reasons it don't works: player crashes. And by some unknown reasons, when we skip removing listeners and replace one tag by another, method getEventListeners() in console of my Chrome shows that old listeners exists on fresh tag too. Possibly react make this magic, possibly it such specifical Chrome (devTools?) bug. To be honest, I did not undestand the problem deeply, but in PR I provided the only working way I found.
There was a problem hiding this comment.
I'm happy to merge the PR and will have a look at fixing it up a bit. Thanks!
There was a problem hiding this comment.
super.componentWillReceiveProps(nextProps)! Exactly!
Follow up to #234 Checks if the element to be rendered will change, if so then remove listeners on willReceiveProps, and add listeners on didUpdate This also fixes a bug where media was pausing when unmounting
Follow up to cookpete/react-player#234 Checks if the element to be rendered will change, if so then remove listeners on willReceiveProps, and add listeners on didUpdate This also fixes a bug where media was pausing when unmounting
Follow up to cookpete/react-player#234 Checks if the element to be rendered will change, if so then remove listeners on willReceiveProps, and add listeners on didUpdate This also fixes a bug where media was pausing when unmounting
Follow up to cookpete/react-player#234 Checks if the element to be rendered will change, if so then remove listeners on willReceiveProps, and add listeners on didUpdate This also fixes a bug where media was pausing when unmounting
Follow up to cookpete/react-player#234 Checks if the element to be rendered will change, if so then remove listeners on willReceiveProps, and add listeners on didUpdate This also fixes a bug where media was pausing when unmounting
Follow up to cookpete/react-player#234 Checks if the element to be rendered will change, if so then remove listeners on willReceiveProps, and add listeners on didUpdate This also fixes a bug where media was pausing when unmounting
Follow up to cookpete/react-player#234 Checks if the element to be rendered will change, if so then remove listeners on willReceiveProps, and add listeners on didUpdate This also fixes a bug where media was pausing when unmounting
Hey!
Big thanks for your work! I use your player in my project and it works good and simply. But when I need to switch videofile to audiofile, all used DOM-listeners gets old and it crashes player normal work.
So... Hope my commit can make your player a little bit better :)